Skip to content
This repository was archived by the owner on May 11, 2026. It is now read-only.

fix: centralize operation error handling#1071

Open
wa0x6e wants to merge 6 commits into
masterfrom
fix/centralize-queryAsync-error-handling
Open

fix: centralize operation error handling#1071
wa0x6e wants to merge 6 commits into
masterfrom
fix/centralize-queryAsync-error-handling

Conversation

@wa0x6e

@wa0x6e wa0x6e commented Feb 14, 2026

Copy link
Copy Markdown
Contributor

This PR refactors the operations/ files, and centralize the error handling in a single place, for easier filtering out of some error code (and also simplify the code)

Summary

  • Centralized error handling for all graphql resolvers via a withErrorHandler wrapper in src/graphql/operations/index.ts
  • All resolver errors are caught, logged, and returned as Promise.reject(new Error('request failed')) — no errors bubble up
  • ER_QUERY_TIMEOUT errors are filtered out from Sentry via IGNORED_ERROR_CODES constant (still logged)
  • Removed redundant try/catch blocks and unused capture/log imports from 12 graphql operation files

Related

Test plan

  • Verify queries still return correct results
  • Trigger a query timeout using the query from the Sentry issue and confirm it is logged but NOT reported to Sentry:
    query {
      votes(first: 10, skip: 46, orderBy: "created", orderDirection: desc) {
        proposal {
          space {
            delegationPortal {
              delegationType
              delegationContract
              delegationNetwork
              delegationApi
            }
          }
        }
      }
    }
  • Verify other errors (DB, JSON.parse, formatting) are still captured in Sentry
  • Verify all resolvers return 'request failed' on error, no raw errors bubble up

wa0x6e and others added 2 commits February 14, 2026 15:55
Fixes SNAPSHOT-HUB-35

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR centralizes MySQL queryAsync error handling in src/helpers/mysql.ts and removes per-resolver try/catch blocks (and related logging/Sentry imports) from several GraphQL operation resolvers to reduce duplication.

Changes:

  • Override Pool.prototype.queryAsync to catch MySQL query errors, log them, optionally report to Sentry, and rethrow a generic "request failed" error.
  • Adjust Bluebird promisifyAll to skip promisifying query so the custom queryAsync can be used.
  • Remove redundant try/catch + capture/log imports from multiple GraphQL operation resolvers.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/helpers/mysql.ts Centralizes queryAsync behavior (custom implementation + Sentry/logging) and alters Bluebird promisification behavior.
src/graphql/operations/votes.ts Removes local try/catch and Sentry/log handling around DB queries.
src/graphql/operations/users.ts Removes local try/catch and Sentry/log handling around DB queries.
src/graphql/operations/subscriptions.ts Removes local try/catch and Sentry/log handling around DB queries.
src/graphql/operations/statements.ts Removes local try/catch and Sentry/log handling around DB queries.
src/graphql/operations/statement.ts Removes local try/catch and Sentry/log handling around DB queries.
src/graphql/operations/roles.ts Removes local try/catch and Sentry/log handling around DB query + mapping.
src/graphql/operations/proposals.ts Removes local try/catch and Sentry/log handling around DB queries.
src/graphql/operations/proposal.ts Removes local try/catch and Sentry/log handling around DB queries.
src/graphql/operations/messages.ts Removes local try/catch and Sentry/log handling around DB queries.
src/graphql/operations/leaderboards.ts Removes local try/catch and Sentry/log handling around DB queries.
src/graphql/operations/follows.ts Removes local try/catch and Sentry/log handling around DB queries.
src/graphql/operations/aliases.ts Removes local try/catch and Sentry/log handling around DB queries.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/helpers/mysql.ts Outdated
Comment thread src/helpers/mysql.ts Outdated
Comment thread src/graphql/operations/roles.ts
wa0x6e and others added 4 commits February 14, 2026 16:15
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@wa0x6e
wa0x6e requested a review from Copilot February 14, 2026 21:22
@wa0x6e wa0x6e changed the title fix: centralize queryAsync error handling in mysql helper fix: centralize operation error handling Feb 14, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +31 to +38
function withErrorHandler(fn) {
return (...args) =>
fn(...args).catch(e => {
if (!IGNORED_ERROR_CODES.includes(e.code)) capture(e);
log.error(`[graphql] ${JSON.stringify(e)}`);
return Promise.reject(new Error('request failed'));
});
}

Copilot AI Feb 14, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The withErrorHandler wrapper calls .catch() on all wrapped functions, but some operations (plugins, skins, validations, strategies) are synchronous and return values directly instead of Promises. Calling .catch() on a non-Promise value will cause a runtime error.

To fix this, the wrapper should handle both synchronous and asynchronous functions. Consider wrapping the function call in Promise.resolve() first, or check if the return value is a Promise before calling .catch().

Copilot uses AI. Check for mistakes.
};

export default Object.fromEntries(
Object.entries(operations).map(([key, fn]) => [key, withErrorHandler(fn)])

Copilot AI Feb 14, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The operations space.ts, spaces.ts, ranking.ts, and vote.ts still have their own try/catch blocks with error handling and logging. This means errors in these operations will be logged twice - once in their own try/catch and again in the withErrorHandler wrapper. Additionally, these operations return Error objects (not Promise.reject), which won't trigger the .catch() in the wrapper, so the centralized error handling won't apply to them at all.

Either remove the try/catch blocks from these files to use only the centralized handler, or remove them from the operations object so they aren't wrapped. If these operations need special error handling (like PublicError), that logic should be incorporated into the centralized handler or kept separate from the wrapper.

Suggested change
Object.entries(operations).map(([key, fn]) => [key, withErrorHandler(fn)])
Object.entries(operations).map(([key, fn]) => {
const skipWrapper = ['space', 'spaces', 'ranking', 'vote'];
return skipWrapper.includes(key) ? [key, fn] : [key, withErrorHandler(fn)];
})

Copilot uses AI. Check for mistakes.
function withErrorHandler(fn) {
return (...args) =>
fn(...args).catch(e => {
if (!IGNORED_ERROR_CODES.includes(e.code)) capture(e);

Copilot AI Feb 14, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The centralized error handler no longer captures context information (args, context, info) that was previously passed to capture() in the individual resolvers. This context is valuable for debugging and error tracking in Sentry. The wrapper has access to the arguments passed to each resolver function through the ...args parameter, but these aren't being passed to capture().

Consider capturing the arguments in the error handler to preserve this debugging context.

Suggested change
if (!IGNORED_ERROR_CODES.includes(e.code)) capture(e);
if (!IGNORED_ERROR_CODES.includes(e.code)) {
const [, resolverArgs, context, info] = args;
capture(e, { args: resolverArgs, context, info });
}

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this make sense as well. maybe we need them?

return (...args) =>
fn(...args).catch(e => {
if (!IGNORED_ERROR_CODES.includes(e.code)) capture(e);
log.error(`[graphql] ${JSON.stringify(e)}`);

Copilot AI Feb 14, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The centralized error logging no longer includes the specific operation name that failed. Previously, each operation logged errors with a specific identifier like [graphql] votes, [graphql] proposals, etc. Now all errors are logged as just [graphql], making it harder to identify which operation failed without examining the full error object.

Consider including the operation name in the log message by adding it as a parameter to the wrapper or extracting it from the function name.

Copilot uses AI. Check for mistakes.
Comment thread src/graphql/operations/index.ts
@wa0x6e
wa0x6e requested a review from ChaituVR February 14, 2026 21:34
Comment on lines -121 to -125
} catch (e: any) {
capture(e, { args, context, info });
log.error(`[graphql] votes, ${JSON.stringify(e)}`);
return Promise.reject(new Error('request failed'));
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can remove catch on vote.ts also right?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also on vp.ts it will ignore the promise.reject messages and will always return request failed?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vote.ts — try/catch with capture
space.ts — try/catch with PublicError handling
spaces.ts — try/catch with PublicError handling
ranking.ts — try/catch with PublicError handling
options.ts — internal run loop with try/catch

return (...args) =>
fn(...args).catch(e => {
if (!IGNORED_ERROR_CODES.includes(e.code)) capture(e);
log.error(`[graphql] ${JSON.stringify(e)}`);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before we know which operation is failing, copilot is correct here

fn(...args).catch(e => {
if (!IGNORED_ERROR_CODES.includes(e.code)) capture(e);
log.error(`[graphql] ${JSON.stringify(e)}`);
return Promise.reject(new Error('request failed'));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe need to figure out how we should handle custom error messages, for example vp.ts, user.ts

function withErrorHandler(fn) {
return (...args) =>
fn(...args).catch(e => {
if (!IGNORED_ERROR_CODES.includes(e.code)) capture(e);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this make sense as well. maybe we need them?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants